Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: several nargo test improvements #6728

Merged
merged 39 commits into from
Dec 12, 2024
Merged

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Dec 6, 2024

Description

Problem

Resolves #4489
Resolves #6724
Resolves #6727

Summary

This PR does a few things:
2. We now use a maximum number of threads to run tests. The default is the system's default (taken from a Rust function) but it can be overwritten with the new --num-threads CLI option (let me know if this should be removed, though, probably always parallelizing is never an issue, unlike in Rust)
3. We now compile all packages and show all errors before starting to run tests (previously the first error would abort the run)
4. Before showing how many tests passed and failed in a package, a list of the tests that failed is shown. This is similar to Rust too. I personally find this useful because otherwise to know which failed you have to scroll up and fine the red "fail" ones, which is a bit time-consuming. But let me know and we could revert this.
5. Output is shown by package. That is, first we show all output for a package, then all output for another package, etc. However, we do run all tests in parallel.
6. Output fron Noir's println isn't printed immediately. Instead, it's captured in a String and associated to the test. Then, if --show-output is given, that output is printed after the test name status. That makes it very easy to see what output each test produced, and output from different tests is never mixed up.
7. For slow tests (those that take more than 30 seconds) we now show the time.

An example of 6:

image

And consider this Noir program:

#[test]
pub fn one() {
    println("one (1)");
    println("one (2)");
    println("one (3)");
}

#[test]
pub fn two() {
    println("two (1)");
    println("two (2)");
    println("two (3)");
}

#[test]
pub fn three() {
    println("three (1)");
    println("three (2)");
    println("three (3)");
}

The new output for nargo test is:

image

The old output was this:

image

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite changed the title Run tests according to the system's parallelism (for now) feat: restore nargo test streaming behavior and restrict number of threads Dec 6, 2024
Copy link
Contributor

github-actions bot commented Dec 6, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.17M
workspace 121.67M
regression_4709 294.72M
ram_blowup_regression 2.44G
private-kernel-tail 208.76M
private-kernel-reset 861.44M
private-kernel-inner 307.68M
parity-root 174.42M

@asterite asterite marked this pull request as ready for review December 9, 2024 14:11
@TomAFrench TomAFrench added the run-external-checks Trigger CI job to run tests on external repos label Dec 9, 2024
@TomAFrench
Copy link
Member

Looks good but I'm seeing a number of new test failures in the run-external-checks tests (the private_rollup_lib tests).

@asterite
Copy link
Collaborator Author

asterite commented Dec 9, 2024

Strange, they pass on my machine.

But also, nargo test is now much slower when there are multiple packages in the workspace, I think it's because they are compiled sequentially.

I'll see if I can compile things in parallel...

@TomAFrench
Copy link
Member

Hmm, we'd need to buffer the test results in that case though wouldn't we otherwise we'd get tests from different packages potentially interleaved.

@asterite
Copy link
Collaborator Author

asterite commented Dec 9, 2024

I managed to do the above (first compile all packages in parallel, then run tests in parallel for each package) but it's still slower than before. For example if there are many, many packages with a very small number of tests, for example 3, and we have 10 threads, we'll just run 3 in parallel in sequential batches.

Maybe running everything in parallel like before is fine? We could still show a summary at the end for each package separately, which is maybe what matters most. What do you think?

@asterite
Copy link
Collaborator Author

asterite commented Dec 9, 2024

Alternatively we could try to just show things sequentially, but still run everything in parallel... but I'm not sure how we could do that. Something like "run everything in parallel, but get the output of the first package first, then the output of the second package, etc.". Maybe we can have a receiver for results: when we get a result, if it's for the package we are interested in we output it, otherwise put it in a HashMap keyed by that package name. Once we finish with a package we first show what we have in the cached output, then continue receiving output from the package, etc... though it will make the code much harder to understand. I'll try it anyway.

@aakoshh
Copy link
Contributor

aakoshh commented Dec 9, 2024

Couple of questions:

  • are you using the same number of threads?
  • what if you disable the printing of results after each test?

I think it wasn’t about going faster but to limit the amount of resource usage; the ticket mentioned running out of memory.

@TomAFrench
Copy link
Member

TomAFrench commented Dec 9, 2024

Yeah, I don't mind if things are a little slower as long as we can run the protocol tests with a simple nargo test (and not needing hacks like artificially reducing the number of cores). We can always improve the speed in future PRs.

@aakoshh
Copy link
Contributor

aakoshh commented Dec 9, 2024

If you consider my earlier pseudo code that set up workers up front, you can imagine setting up a pipeline where you feed tests in order into the work queue, so it will be package A first then package B, and in another component you tell up front how many results are to be expected from each package. That component gets the results in a channel, and if they are out of order it buffers them, if it’s for the current item it shows them, when it reaches the expected number of reports it shows the summary and starts outputting the next one from the buffer

@asterite
Copy link
Collaborator Author

asterite commented Dec 9, 2024

@aakoshh Right, I looked at your code to see if I could use it, because it seems what we'd need to do in the end. The problem is that to get the tests to run we need to compile a package. In the pseudocode packages are compiled sequentially and that's what's slow.

However, I'll at least change things so that all packages are compiled in parallel. There's still a slowdown because we don't run tests from different packages in parallel. If the issue was too many threads and out of memory, using worker threads solved this but then I suggested running packages sequentially and that's what caused the additional slowdown. What I'm saying is that we could revert that functionality, get rid of the "out of memory", and still have things work really fast.

@asterite
Copy link
Collaborator Author

asterite commented Dec 9, 2024

Well, running tests for noir-contracts (with oracles failing) now takes 6 seconds vs. 2, so maybe it's not a big deal. I'll just push a commit that parallelizes the compilation.

@TomAFrench
Copy link
Member

Or ir could be after, like now, but maybe precede it with "standard outfor for test:"

I would say this.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

tooling/nargo/src/ops/test.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
tooling/nargo_cli/src/cli/test_cmd.rs Outdated Show resolved Hide resolved
@asterite
Copy link
Collaborator Author

asterite commented Dec 11, 2024

One more thing I did: I noticed that if we have many packages, we first print how many tests are for each package, then print package tests sequentially. However, it's probably nicer if we print the package tests count and the tests all together. That is, instead of this:

[one] Running 2 test functions
[two] Running 1 test function
[one] Testing a... ok
[one] Testing b... ok
[one] 2 tests passed
[two] Testing c... ok
[two] 1 test passed

Show it like this:

[one] Running 2 test functions
[one] Testing a... ok
[one] Testing b... ok
[one] 2 tests passed
[two] Running 1 test function
[two] Testing c... ok
[two] 1 test passed

At this point I'm wondering if we actually need to prefix all the output with the package name. We could maybe print it at the beginning and the end, but not for every test. Thoughts?

(I don't have a strong opinion about this, but Rust more or less does that)

@TomAFrench
Copy link
Member

At this point I'm wondering if we actually need to prefix all the output with the package name. We could maybe print it at the beginning and the end, but not for every test. Thoughts?

Let's keep it for now. This is consistent with other packages and we have quite a few changes in this PR already.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@asterite asterite added this pull request to the merge queue Dec 12, 2024
Merged via the queue into master with commit 1b0dd41 Dec 12, 2024
78 of 81 checks passed
@asterite asterite deleted the ab/nargo-test-improvements branch December 12, 2024 02:10
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 12, 2024
)

chore: Try replace callstack with a linked list (noir-lang/noir#6747)
chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 12, 2024
chore: Try replace callstack with a linked list (noir-lang/noir#6747)
chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-external-checks Trigger CI job to run tests on external repos
Projects
None yet
3 participants